Codecov Report

All modified and coverable lines are covered by tests ✅

... and 5 files with indirect coverage changes

Am I right that this PR adds a specialized function to calculate $\langle O_1(i) O_2(j) \rangle$ for two sites i, j in the same row/column? Then the vertical version can be just the application of the horizontal one on rotated PEPS + CTMRGEnv.

In addition, sometimes its hard to split $O_1 O_2$ into two tensors (without auxiliary legs), such as $c^\dagger_{i\sigma} c_{j\sigma}$, or the SU(2)-symmetric version of S_exchange. Should we take this into account?

This was precisely the original discussion in one of the earlier PRs. I would prefer if we default to giving the two-body operator and split it up ourselves, ie always work with an auxiliary leg, because I don't like code that only works for non-symmetric cases

Looking at this a bit more in depth: can we just mimick the MPSKit functions from this file?

For example, I like that in MPSKit there is a distinction between specifying two Ints, which gives you a single value, and one Int and a range, which gives you all values you requested.
A straightforward generalization would be i::CartesianIndex, j::CartesianIndex giving a single value (basically an expectation value), while i::CartesianIndex, j::Range{CartesianIndex} would give you all values inbetween.
Note that IIRC this works: CartesianIndex(1, 1):CartesianIndex(0, 1):CartesianIndex(21, 1).

For having the option to have different bra and ket, I'm happy to generalize the MPSKit function too: what do you think about MPSKit.correlator((bra, ket), ...) for that? Alternatively there are many parts of MPSKit that have bra, operator, ket, envs as argument orders, so I would also be fine with bra, O12, i, j, ket, envs.

For the implementation I have more or less the same remarks, is there an option to define this in a similar way as MPSKit, using transfer matrices? I am guessing there might be some missing transfer_left and transfer_right functions to pass the string, but otherwise this should be more or less the same. The main advantage being that this avoids having to define contractions in too many places, and more easily generalizes to things with other numbers of legs.
In an ideal world, we would define the MPSKit correlator in terms of generic functions that can be overloaded here, but I am fine not trying to merge these two for now.

This was precisely the original discussion in one of the earlier PRs. I would prefer if we default to giving the two-body operator and split it up ourselves, ie always work with an auxiliary leg, because I don't like code that only works for non-symmetric cases

That's mainly why I added both options, because this way it works for symmetric tensors as well in the case where the operators have a product structure. If we want to have the correlation function of e.g. S_zz, we otherwise would have to give $$Sz \otimes Sz$$ as argument, which the code would then internally split, which feels a little bit redundant (even though the extra computational cost will only be marginal).

If we wouldn't allow product structure operators here, then maybe we also shouldn't allow them when we generalize this function to InfinitePartitionFunctions. For the latter case, in the context of calculating the expectation value of a LocalOperator of an InfinitePEPO, which would internally use 'excited' partition functions, I would also like to keep the option open for three-body operators. Then we would anyway have to give up the restriction of only allowing one (and only one) auxiliary leg.

TL;DR I would prefer to keep both options open, but wouldn't mind to restrict it to 2-site operators or to make the function where we use 2 one-site operators with auxiliary legs an internal function.

Looking at this a bit more in depth: can we just mimick the MPSKit functions from this file?

For example, I like that in MPSKit there is a distinction between specifying two Ints, which gives you a single value, and one Int and a range, which gives you all values you requested. A straightforward generalization would be i::CartesianIndex, j::CartesianIndex giving a single value (basically an expectation value), while i::CartesianIndex, j::Range{CartesianIndex} would give you all values inbetween. Note that IIRC this works: CartesianIndex(1, 1):CartesianIndex(0, 1):CartesianIndex(21, 1).

For having the option to have different bra and ket, I'm happy to generalize the MPSKit function too: what do you think about MPSKit.correlator((bra, ket), ...) for that? Alternatively there are many parts of MPSKit that have bra, operator, ket, envs as argument orders, so I would also be fine with bra, O12, i, j, ket, envs.

For the implementation I have more or less the same remarks, is there an option to define this in a similar way as MPSKit, using transfer matrices? I am guessing there might be some missing transfer_left and transfer_right functions to pass the string, but otherwise this should be more or less the same. The main advantage being that this avoids having to define contractions in too many places, and more easily generalizes to things with other numbers of legs. In an ideal world, we would define the MPSKit correlator in terms of generic functions that can be overloaded here, but I am fine not trying to merge these two for now.

Is there a reason why the order is bra, operator, ket,... in MPSKit? In PEPSKit we have for an InfinitePartitionFunction the order ket, operator, bra for an InfiniteSquareNetwork, and that might make things more confusing. So I think I would prefer ket, O, i, j, bra, envs (regardless of whether we only allow two-site operators).

For the implementation: I indeed tried to mimic MPSKit as much as possible. I also implemented the variant of the TransferMatrix in this file on my fork, but I noticed that it is way less efficient then the other implementation, mainly because we have to create a 10-leg tensor here. I'll test tomorrow whether it becomes more efficient if I just take the power of the transfer matrix instead of taking the eigenvalue decomposition, but I doubt it... I do agree that it would be useful to specify either a number or a range, and in the former case the transfermatrix implementation might be more efficient.

Just making sure we understand each other: the MPSKit implementation does not actually build the transfer matrix, it makes an object that holds the tensors that represent it, and the transfer functions only implement the actions of that transfer operator. In other words, it should more or less be the same implementation you have, just organized differently

I now changed the code to be more closely related to how MPSKit does it, by defining transfer matrices and the function transfer_left. For now we don't need a transfer_right, so the implementation is somewhat simpler. I kept the ability to use operators without auxiliary leg (in a tuple), since it does not require an extra function now, just a slightly different implementation of transfer_left.

After thinking about it a little bit more, the order bra, O12, i, j, ket, envs makes a lot of sense, so I changed the order to that one. It now accepts only a CartesianIndex for i, and a CartesianIndices, a CartesianIndex (in line with MPSKit, returning a single element), and an Int specifying the distance, like @Yue-Zhengyuan suggested.

Any additional comments or suggestions are very much appreciated.

The VUMPS tests are now failing since I am overloading transfer_left functions to make sure they don't apply conj to the bottom layer, whereas the VUMPS functions do need that. We can either resolve this by reverting to a struct called HorizontalTransferMatrix (which doesn't necessarily need to be derived from an abstract type), add a field to AbstractTransferMatrix to signify whether we want to have the twists and conjugations (Default false), or manually conjugate the environment tensors to be compatible with the current transfer_left. I have a slight preference for option 1, but opinions and alternative options are welcome.

You can check the correlation length function for how this was calculated before, see

Sure, go ahead

I updated the implementation to more closely resemble MPSKit's version, and tried to be a bit more generic with the input types. I also added some tests to ensure that the rotation is correctly handled, and that the range does not need to be contiguous (this is important when dealing with mixed physical spaces, where an operator might not even fit on each site).

I repurposed the MPSKit.FiniteMPO to convert the operator to things that can be handled by the contractions, I think it is fair to accept anything that can be converted to that.

Let me know what you think about this.

I'll try to use these functions for an iPEPS and CTMRGEnv with large bond dimensions and see if the memory usage is improved over the generic expectation_value (#163).

What happened with the codedev check? It reports 0% test coverage.

What happened with the codedev check? It reports 0% test coverage.

Sometimes it needs to rerun after all other checks have passed. Now it's back to 100% test coverage.